Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(e2e): test sync percentage #2264

Merged
merged 5 commits into from
Feb 16, 2024

Conversation

yonadaa
Copy link
Contributor

@yonadaa yonadaa commented Feb 14, 2024

Tests that the sync progress is 100 when live

Copy link

changeset-bot bot commented Feb 14, 2024

⚠️ No Changeset found

Latest commit: d9b04ea

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@yonadaa yonadaa marked this pull request as ready for review February 14, 2024 15:58
@yonadaa yonadaa requested review from alvrs and holic as code owners February 14, 2024 15:58
@yonadaa
Copy link
Contributor Author

yonadaa commented Feb 15, 2024

It would be nice to test the percentage increase from 0, 50, 100, etc..., but because of how chunk sizes work we'd need to generate over 5000 logs, which is too slow on the test set up.

Tested manually and it does though!

@@ -3,4 +3,6 @@ import { Page, expect } from "@playwright/test";
export async function waitForInitialSync(page: Page) {
const syncStep = page.locator("#sync-step");
await expect(syncStep).toHaveText("live");
const syncProgress = page.locator("#sync-percentage");
await expect(syncProgress).toHaveText("100");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to test for this?

I kinda wonder about keeping the "live" indicator (caught up) separate from the percentage synced, so you can e.g. hide your load screen (because "live") but potentially show how "up to date" you are (percentage)

Copy link
Contributor Author

@yonadaa yonadaa Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense - in that case I think we can remove this test. createStoreSync.test.ts does it more directly


const address = "0xad97505a508daf984fe60302109e0115e544b267";

describe("createStoreSync", () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is here because we haven't created all the scaffolding (anvil, database, etc.) in store-sync tests yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - store-sync doesn't have a contracts folder, execa, etc. I kind of like that it is more pure but we do risk e2e getting bloated with tests that are not fully "end to end"

Copy link
Member

@holic holic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to test the percentage increase from 0, 50, 100, etc..., but because of how chunk sizes work we'd need to generate over 5000 logs, which is too slow on the test set up.

We could make this configurable so it's easier to test. But no need to do that here!

@yonadaa yonadaa merged commit a0cd05c into kooshaba/hydration-updates Feb 16, 2024
11 checks passed
@yonadaa yonadaa deleted the yonadaaa/hydration-updates-tests branch February 16, 2024 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants